fix(appsec): report all tags on the service entry span instead of the local root span #14210
Conversation
florentinl
force-pushed
the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
from
cf74a41 to
4453c1f
Compare
|
|
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 273 ± 3 ms. The average import time from base is: 275 ± 2 ms. The import time difference between this PR and base is: -2.3 ± 0.1 ms. Import time breakdownThe following import paths have shrunk:
|
florentinl
force-pushed
the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
from
4453c1f to
95a54a8
Compare
Performance SLOsCandidate: florentinl/APPSEC-58532/report-tags-on-service-entry-span (a048425) 🔵 No Baseline Data (24 suites)🔵 coreapiscenario - 12/12 (2 unstable)🔵 No baseline data available for this suite
|
florentinl
force-pushed
the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
22 times, most recently
from
bd9d326 to
e7f2dde
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
florentinl
changed the title
florentinl
force-pushed
the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
from
e7f2dde to
050c4b5
Compare
florentinl
force-pushed
the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
4 times, most recently
from
d548fb5 to
c14225e
Compare
florentinl
changed the title
florentinl
removed
the
changelog/no-changelog
florentinl
force-pushed
the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
5 times, most recently
from
709bc4c to
2fc795b
Compare
florentinl
force-pushed
the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
from
2fc795b to
784ccf0
Compare
|
Great Work! |
florentinl
force-pushed
the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
from
898ffbc to
a048425
Compare
florentinl
deleted the
florentinl/APPSEC-58532/report-tags-on-service-entry-span
branch
## Description This is an omission from PR: #14210 . The `track_user` function must pass to tag the user on to the `set_user` helper. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
… local root span (#14210) ## Motivation In specific cases where we have an inferred span (belonging to an inferred service) as the parent of the framework instrumented web request (the 'web' span), Appsec is enabled on the inferred service instead of the current service. This is because we always use set tags and metrics on the root span using the `span._local_root` helper. This leads to an incorrect reporting of the appsec enablement status, and security signals showing up on spans of a different service. Note: - The only type of inferred span in my understanding are API Gateways. They can be created either through the AWS Lambda python layer and are of type 'http' or are inferred by dd-trace-py and are of type 'web'. - Appsec through the tracer in lambda is not generally available yet, and the extension correctly handles this case. So no problems for now. But is necessary for in tracer enablement of appsec. ## Changes - Add a helper on spans to retrieve the top level span of a given service - Modify the appsec logic to always report on the service entry span and add a failsafe to query the entry span of the current span or current root span to get a handle on a span in any situation. - Modify the tests to look for the entry span instead of the root span Notes: - for IAST, we still need to check the root span in some cases because of the `_DD_IAST_USE_ROOT_SPAN` flag. This setting also needs additional manual handling in `report_stack` as reporting to root span is not the default anymore. - The contrib_appsec tests contain shell injections (that we ensure are detected by appsec). I included that path to the codeql bypass file. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
Motivation
In specific cases where we have an inferred span (belonging to an inferred service) as the parent of the framework instrumented web request (the 'web' span), Appsec is enabled on the inferred service instead of the current service.
This is because we always use set tags and metrics on the root span using the
span._local_roothelper.This leads to an incorrect reporting of the appsec enablement status, and security signals showing up on spans of a different service.
Note:
Changes
Add a helper on spans to retrieve the top level span of a given service
Modify the appsec logic to always report on the service entry span and add a failsafe to query the entry span of the current span or current root span to get a handle on a span in any situation.
Modify the tests to look for the entry span instead of the root span
Notes:
_DD_IAST_USE_ROOT_SPANflag. This setting also needs additional manual handling inreport_stackas reporting to root span is not the default anymore.Checklist
Reviewer Checklist